Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add short_item_threshold config option #5228

Merged

Conversation

123vivekr
Copy link
Contributor

Allow custom short item threshold values via config

Closes #5225

@calebcartwright
Copy link
Member

Awesome thank you! I forgot to include one very important step, my apologies. We should also include some test cases with the snippets provided in #5225 to show the new config option in use.

I think it should suffice to add two input files (one with the default value for the new opt, and one with an overridden value) under tests/source/configs and then their corresponding resultant files under tests/target/configs

@123vivekr
Copy link
Contributor Author

It is done. Thanks a lot for your help @calebcartwright

@123vivekr 123vivekr force-pushed the custom_short_item_threshold branch from deb2ec3 to 950e004 Compare February 28, 2022 06:53
Configurations.md Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thank you! Couple minor items left inline for you

@123vivekr 123vivekr force-pushed the custom_short_item_threshold branch from 950e004 to 2c05365 Compare March 2, 2022 08:45
@calebcartwright
Copy link
Member

I almost feel like I need to apologize for this question, but after reflecting a bit more I'm not sure short_item is really the best moniker. I say that both because "item" is a very overloaded term in general but has specific meanings in Rust in particular including in other configuration options, and also because the option name doesn't really convey what it does in and of itself.

Curious what you, and anyone else, think @123vivekr? Would something like... short_array_element_threshold be less ambiguous?

@ytmimi
Copy link
Contributor

ytmimi commented Mar 4, 2022

Would something like... short_array_element_threshold be less ambiguous?

I think the name is less ambiguous, but I wonder if it fully conveys its purpose. A long one, but what about simple_array_element_width_threshold. I also wonder if we should add more to the description in Configurations.md. Maybe its worth defining what a "simple" array element is and explain that this value controls Mixed array element formatting.

@calebcartwright
Copy link
Member

calebcartwright commented Mar 5, 2022

I also wonder if we should add more to the description in Configurations.md.

Agreed, I was saying the same in #5228 (comment) though it may not have been clear I was asking for both description updates and examples.

How about something along the lines of this (please feel free to wordsmith):

The length limit of an array element in order for the element to be considered "short".

The layout of an array's elements is dependent on the length of each of those elements. If the length of every element in an array is below this threshold (all elements are "short") then the array can be formatted in the mixed/compressed style, but if any one element has a length that exceeds this threshold then the array elements will have to be formatted vertically

Allow custom short item threshold values via config
@123vivekr 123vivekr force-pushed the custom_short_item_threshold branch from 2c05365 to 6cf7e3b Compare March 20, 2022 17:44
@123vivekr
Copy link
Contributor Author

@calebcartwright @ytmimi I went with short_array_element_width_threshold which sounded more accurate.

@calebcartwright I have also added your description to the Configurations.md, with a few changes to simplify it.

Do let me know if there is anything I could help with. :)

@calebcartwright
Copy link
Member

Excellent, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Customizable SHORT_ITEM_THRESHOLD
3 participants